-
Notifications
You must be signed in to change notification settings - Fork 587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add --ignore-states flag for ignoring findings with specific fix states #1473
Conversation
Hey @jhebden-gl, thank you very much for this patch! We're talking this over and we might want to go in a slightly different direction. What do you think if instead of adding --ignore-wontfix, we moved the ignore functionality to a --show option that takes a list: --show fixed,wontfix,notfixed This might help us shrink the list of different flags out there, especially if/when we add more states. Would you be able to join one of our community meetings to discuss? Might be nice to talk it over live. (https://github.com/anchore/grype#join-our-community-meetings) |
Hey @tgerla! Thanks for taking a look and for the feedback.
I think this is a great idea, and totally hear you on cluttering up the number of options. I think this approach is also less opinionated, so users can do their own filtering. I'm happy to rework the MR to support that, if that's the direction y'all would like to take.
I'd be totally happy to jump on a community meeting and talk over some options. I'm based in NSW, Australia, though - so the time I'm seeing for the next meeting is at around 2AM local for me, which might make it a bit tricky for me to attend. I'm also happy to chat about this further on this MR, or via another async method if that works. |
@jhebden-gl Thanks very much - we'd really appreciate it if you could update this change to add a |
b4c6b5c
to
cc6319f
Compare
I've updated the branch & PR to extend this to enable filtering on specific states, provided as a list. If it's OK, I opted to use I've tested a few different combinations against a known image with lots of fixed & wont-fix findings and this also does not interfere with the existing I've also updated the README and added a little helper to get the known fix states, so this could be shown in the Let me know if you'd like anything changed or if you have any other feedback, and thanks for the review & feedback so far. |
62f13df
to
44521a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This @jhebden-gl thanks very much for the contribution! I've made a couple of comments. If you'd like, we can take it from here, and make those changes on top of your branch, or you can update the PR. Let me know which way you'd like to proceed, and thanks again!
cmd/grype/cli/commands/root.go
Outdated
case grypeDb.UnknownFixState, grypeDb.FixedState, grypeDb.NotFixedState, grypeDb.WontFixState: | ||
opts.Ignore = append(opts.Ignore, match.IgnoreRule{FixState: ignoreState}) | ||
default: | ||
log.Warnf("ignoring unknown fix state %s for --ignore-states", ignoreState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should return an error instead of logging a warning; the CLI should fail and explain why if the user invokes it with invalid config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this on the latest commit, and I've also moved this (and the other ignore state handling) up so they happen prior to downloading the DB and running the scan, to enable a fast-feedback loop if a user supplies an invalid fix state (rather than waiting for the DB update & scan to run before they are made aware of the their typo 😄)
cmd/grype/cli/options/grype.go
Outdated
@@ -103,6 +104,11 @@ func (o *Grype) AddFlags(flags clio.FlagSet) { | |||
"ignore matches for vulnerabilities that are fixed", | |||
) | |||
|
|||
flags.StringArrayVarP(&o.IgnoreStates, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if it could be invoked with a comma-separate list, like --ignore-states not-fixed,wont-fix
, but right now I seem to have to pass --ignore-states not-fixed --ignore-states wont-fix
. --catalogers
is handled this way in Syft, code at https://github.com/anchore/syft/blob/f6c805797715b3befbf65c291f466f6fbd94fc0b/cmd/syft/cli/options/catalog.go#L100-L104.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this on the latest commit by moving this parameter to a string and adding a helper for splitting strings on commas and filtering empty entries. I've also added a test for the helper. With these changes in place, the parameter now processes a comma-separated list of states.
Signed-off-by: James Hebden <jhebden@gitlab.com>
44521a0
to
5e41415
Compare
Thanks for the review & feedback! I've addressed the latest comments and tested them locally. Let me know if you have any further feedback. |
…nore states comma-separated Signed-off-by: James Hebden <jhebden@gitlab.com>
5e41415
to
d20c4dc
Compare
Thanks @jhebden-gl! I'm taking a look today. |
Signed-off-by: Will Murphy <will.murphy@anchore.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice job 🙌
* main: (137 commits) chore(deps): bump actions/checkout from 4.1.0 to 4.1.1 (#1564) Add --ignore-states flag for ignoring findings with specific fix states (#1473) feat: update go-sarif library to use latest release (#1563) bump clio to get stderr reporting fix (#1561) chore(deps): bump github.com/gabriel-vasile/mimetype from 1.4.2 to 1.4.3 (#1558) chore(deps): bump github.com/charmbracelet/lipgloss from 0.9.0 to 0.9.1 (#1557) Add checksum signing (#1535) chore(deps): bump golang.org/x/net from 0.16.0 to 0.17.0 (#1554) feat: disable CPE-based matching for GHSA ecosystems by default (#1412) chore(deps): bump github.com/google/go-cmp from 0.5.9 to 0.6.0 (#1552) chore(deps): update Syft to v0.93.0 (#1550) chore(deps): bump gorm.io/gorm from 1.25.4 to 1.25.5 (#1547) chore(deps): bump github.com/charmbracelet/lipgloss from 0.8.0 to 0.9.0 (#1548) chore(deps): bump github.com/hashicorp/go-getter from 1.7.2 to 1.7.3 (#1549) chore(deps): bump ossf/scorecard-action from 2.2.0 to 2.3.0 (#1544) fix: empty descriptor name and version (#1542) chore: removes unnecessary conditional (#1539) chore(deps): bump github.com/gkampitakis/go-snaps from 0.4.10 to 0.4.11 (#1533) chore(deps): update Syft to v0.92.0 (#1527) chore(deps): update bootstrap tools to latest versions (#1524) ...
Similar to the
--only-fixed
and--only-unfixed
flags, this PR adds--ignore-wontfix
to specifically ignore findings where the software vendor (i.e. Red Hat or Debian) has stated a vulnerability will not be fixed.Typically findings of this nature represent vulnerabilities which have been assessed & analysed by the software vendor as either not requiring a fix due to the way the software has been built or packaged by the vendor, or where the vulnerability is not exploitable in the default configuration. In the case of some images (i.e. Red Hat's
ubi8/ruby31
image) this can represent hundreds of findings for an image which do not represent exploitable vulnerabilities. More importantly, users may want to specifically filter these out as they will not be fixed, and therefore are not actionable.This differs slightly from the
--ignore-unfixed
flag and the filtering in place, as the--ignore-unfixed
ignores both findings which will not be fixed, but also findings which have not been fixed yet. This distinction is important when reporting findings as there is benefit in a container scanner alerting users to vulnerabilities they will potentially have to report on or track through to fix availability, but for most teams there is typically no benefit to reporting findings which are not exploitable or impactful as assessed by the vendor, and especially as they are not actionable given no fix is going to be made available.I opted to add
--ignore-wontfix
as this wording seems clearer about the intent of the flag, and did not add a generic flag to ignore arbitrary advisory statuses as this seemed like the smallest possible change.I have also moved some of the ignore/filtering flags in
cmd/grype/cli/legacy/root.go
into a separatesetIgnoreFlags
function which is called bysetRootFlags
to keep the linter happy with the size of the function.I've confirmed this works as intended by scanning the
registry.access.redhat.com/ubi8/ruby-31
image. Scanning with and without the--ignore-wontfix
flag produces a delta of 533 wontfix findings, which are correctly removed from the output of thegrype
scan with--ignore-wontfix
.